ImGuiIntegration: add tests, support ImDrawCmd::VtxOffset #90

pezcode · 2022-01-12T19:51:43Z

Closes #82.

I'm not 100% sure if the version/extension checks are correct this way. Info was taken from https://doc.magnum.graphics/magnum/classMagnum_1_1GL_1_1Mesh.html#aaa49afa2c76cc12b8402418a6e31923f.

~~This also needs some more testing on GLES/WebGL, hence a draft PR.~~

pezcode · 2022-01-14T21:12:19Z

I added a small test window to visually check for correct handling of VtxOffset, taken from here.

If all goes well, it should look something like this with no obvious glitches:

(screenshot taken on Chrome 97, with WebGL Draft Extensions enabled)

codecov · 2022-01-14T21:49:10Z

Codecov Report

Merging #90 (ac74cf6) into master (26e7ca9) will decrease coverage by 0.04%.
The diff coverage is 80.00%.

@@            Coverage Diff             @@
##           master      #90      +/-   ##
==========================================
- Coverage   77.20%   77.16%   -0.05%     
==========================================
  Files          21       21              
  Lines         939      946       +7     
==========================================
+ Hits          725      730       +5     
- Misses        214      216       +2

Impacted Files	Coverage Δ
src/Magnum/ImGuiIntegration/Context.h	`100.00% <ø> (ø)`
src/Magnum/ImGuiIntegration/Context.cpp	`89.30% <80.00%> (-0.83%)`	⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 26e7ca9...ac74cf6. Read the comment docs.

pezcode · 2022-01-19T17:59:59Z

Forgot to mention: this unconditionally requires imgui 1.71. Since that version is considered obsolete (ie. no official backward compatibility in the API), I'd say that's acceptable. If you'd prefer to support older versions, we can #ifdef around this as well.

src/Magnum/ImGuiIntegration/Context.cpp

ImTextureID is void*

pezcode · 2022-02-05T22:36:43Z

I added integration tests for renderFrame() to make sure it correctly handles scissor rect, texture IDs, index offset and vertex offset (if supported). Hopefully makes your Vulkan porting easier in the future, too 🌋 The visual test got removed because it's now unnecessary.

All the scaffolding was taken from the Magnum Shader tests. Some special-casing (iOS?) may not be needed, I wasn't too sure about the interactions there.

pezcode · 2022-02-05T23:14:43Z

Not sure what's happening to Github, it just completely skipped running CI 💩

pezcode · 2022-02-06T11:59:57Z

The linux-gles tests seem to be the only ones that run ctest without -E GLTest. Can we enable the GL tests in other CIs without too much breaking? I assume they're disabled for a good reason, but running the ImGui drawFrame() tests on a few different platforms would be useful.

mosra

Thank you! Tests look great 👍

Can we enable the GL tests in other CIs without too much breaking?

The GL tests run against SwiftShader, which is the only software GL implementation I was able to get working there, and it's GLES-only. I could enable it on macOS as well as Magnum builds got that recently already, on Windows I don't have any existing SwiftShader build to copy from yet, so there it will take some more effort.

After I manage to tag a release I plan to drop support for Ubuntu 16.04, and then I could try adding a Mesa llvmpipe build, to have an ability to test the full desktop GL. But that's still quite far ahead.

src/Magnum/ImGuiIntegration/Test/CMakeLists.txt

src/Magnum/ImGuiIntegration/Test/ContextGLTest.cpp

src/Magnum/ImGuiIntegration/Context.h

pezcode · 2022-02-06T16:12:11Z

Thanks for the detailed review ☺️

I think I fixed everything, except for replacing TgaImporter with PngImporter. The effort of adding and building magnum-plugins on all CIs just for the ImGuiIntegration GL tests didn't seem worth it considering the test images are 2.5KB combined. But if you really have a strong preference for PNGs, I can add it.

mosra · 2022-02-06T16:29:06Z

Since it's a UI library, I anticipate the test image corpus to grow over time, especially if/when we switch to our own text rendering, or to Magnum's own theme, for example. With small images TGAs are acceptable, but when they grow larger it stops being practical due to the huge file sizes. (Not to mention the lack of software support makes dealing with them kinda annoying, especially here in the browser.)

adding and building magnum-plugins on all CIs

Just on the ES build, in unix-desktop-gles.sh, where the GL tests are run ;) If you pick StbImageImporter (and not the full PngImporter with the external dependencies), apart from the time it takes to clone the repo & run cmake, the build time would be comparable to build time of the TgaImporter. Nothing else from the plugins repo will get built, I made it so exactly this use case is as efficient as it can be.

You can reuse the relevant piece from unix-desktop.sh, just drop the DART-related stuff:

magnum-integration/package/ci/unix-desktop.sh

Lines 51 to 64 in 26e7ca9

    
           # Magnum Plugins 
        
           git clone --depth 1 https://github.com/mosra/magnum-plugins.git 
        
           cd magnum-plugins 
        
           mkdir build && cd build 
        
           cmake .. \ 
        
               -DCMAKE_CXX_FLAGS="$CMAKE_CXX_FLAGS" \ 
        
               -DCMAKE_INSTALL_PREFIX=$HOME/deps \ 
        
               -DCMAKE_INSTALL_RPATH=$HOME/deps/lib \ 
        
               -DCMAKE_BUILD_TYPE=$CONFIGURATION \ 
        
               -DWITH_ASSIMPIMPORTER=$WITH_DART \ 
        
               -DWITH_STBIMAGEIMPORTER=$WITH_DART \ 
        
               -G Ninja 
        
           ninja install 
        
           cd ../..

Using rather basic integration tests by drawing a few shapes and then comparing the framebuffer content against known correct output. I hope the thresholds don't blow up in my face on WebGL.

…ader

mosra

🎉

pezcode · 2022-02-07T12:23:34Z

Just realized the TgaImporter is still being built, as well as AnyImageImporter on CIs that don't run the GL tests. If you're not in a rush, I'll fix it sometime this week. 🙇

mosra · 2022-02-08T13:03:33Z

Merged as 26e7ca9...7931348, thanks for the reminder -- I changed the commits to enable the relevant plugins only for the ES CI builds.

As of b29aeee there's also a GLES3 macOS build, I'll eventually add Windows variants once Magnum has them.

pezcode force-pushed the imgui-vtxoffset branch from d974856 to 5bd9de3 Compare January 14, 2022 21:04

pezcode marked this pull request as ready for review January 14, 2022 21:12

pezcode mentioned this pull request Jan 19, 2022

ImGuiIntegration: use provided index offset #88

Closed

pezcode force-pushed the imgui-vtxoffset branch from 5bd9de3 to ee8c63c Compare February 2, 2022 10:43

mosra added this to the 2022.0a milestone Feb 3, 2022

mosra reviewed Feb 3, 2022

View reviewed changes

src/Magnum/ImGuiIntegration/Context.cpp Outdated Show resolved Hide resolved

src/Magnum/ImGuiIntegration/Context.cpp Show resolved Hide resolved

ImGuiIntegration: these can be static_cast

6496cbe

ImTextureID is void*

pezcode force-pushed the imgui-vtxoffset branch from ee8c63c to b484d1f Compare February 5, 2022 22:40

pezcode changed the title ~~ImGuiIntegration: add support for ImDrawCmd::VtxOffset~~ ImGuiIntegration: add tests, support ImDrawCmd::VtxOffset Feb 5, 2022

pezcode force-pushed the imgui-vtxoffset branch 2 times, most recently from 06b0194 to c0a987b Compare February 6, 2022 11:40

mosra reviewed Feb 6, 2022

View reviewed changes

pezcode force-pushed the imgui-vtxoffset branch from c0a987b to b654307 Compare February 6, 2022 16:08

pezcode added 3 commits February 6, 2022 18:29

ImGuiIntegration: test output of drawFrame()

0382b16

Using rather basic integration tests by drawing a few shapes and then comparing the framebuffer content against known correct output. I hope the thresholds don't blow up in my face on WebGL.

ImGuiIntegration: add support for ImDrawCmd::VtxOffset

1da9aae

ImGuiIntegration: document large mesh support

d20f73f

pezcode force-pushed the imgui-vtxoffset branch from b654307 to d20f73f Compare February 6, 2022 17:29

ImGuiIntegration: adapt image comparison error thresholds for SwiftSh…

ac74cf6

…ader

mosra approved these changes Feb 6, 2022

View reviewed changes

mosra added the changelog mention added label Feb 8, 2022

mosra closed this Feb 8, 2022

mosra mentioned this pull request Feb 8, 2022

ImGui integration does not support drawing lots of elements (ImGuiBackendFlags_RendererHasVtxOffset) #82

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ImGuiIntegration: add tests, support ImDrawCmd::VtxOffset #90

ImGuiIntegration: add tests, support ImDrawCmd::VtxOffset #90

pezcode commented Jan 12, 2022 •

edited

Loading

pezcode commented Jan 14, 2022

codecov bot commented Jan 14, 2022 •

edited

Loading

pezcode commented Jan 19, 2022

pezcode commented Feb 5, 2022

pezcode commented Feb 5, 2022

pezcode commented Feb 6, 2022

mosra left a comment •

edited

Loading

pezcode commented Feb 6, 2022

mosra commented Feb 6, 2022 •

edited

Loading

mosra left a comment

pezcode commented Feb 7, 2022

mosra commented Feb 8, 2022

ImGuiIntegration: add tests, support ImDrawCmd::VtxOffset #90

ImGuiIntegration: add tests, support ImDrawCmd::VtxOffset #90

Conversation

pezcode commented Jan 12, 2022 • edited Loading

pezcode commented Jan 14, 2022

codecov bot commented Jan 14, 2022 • edited Loading

Codecov Report

pezcode commented Jan 19, 2022

pezcode commented Feb 5, 2022

pezcode commented Feb 5, 2022

pezcode commented Feb 6, 2022

mosra left a comment • edited Loading

Choose a reason for hiding this comment

pezcode commented Feb 6, 2022

mosra commented Feb 6, 2022 • edited Loading

mosra left a comment

Choose a reason for hiding this comment

pezcode commented Feb 7, 2022

mosra commented Feb 8, 2022

pezcode commented Jan 12, 2022 •

edited

Loading

codecov bot commented Jan 14, 2022 •

edited

Loading

mosra left a comment •

edited

Loading

mosra commented Feb 6, 2022 •

edited

Loading